Forward-compatibly deny drops in constants if they could actually run. #43932

eddyb · 2017-08-17T10:11:48Z

This is part of #40036, specifically the checks for user-defined destructor invocations on locals which may not have been moved away, the motivating example being:

const FOO: i32 = (HasDrop {...}, 0).1;

The evaluation of constant MIR will continue to create 'static slots for more locals than is necessary (if Storage{Live,Dead} statements are ignored), but it shouldn't be misusable.

r? @nikomatsakis

arielb1 · 2017-08-17T10:35:23Z

src/librustc_mir/transform/qualify_consts.rs

+            // Deny *any* live drops anywhere other than functions.
+            if self.mode != Mode::Fn {
+                // HACK(eddyb) Emulate a bit of dataflow analysis,
+                // conservatively, that drop elaboration will do.


This is not really "drop elaboration" dataflow - drop elaboration does not know that None has no destructor - but rather "const qual" dataflow.

Right, it's a combination of the two.

eddyb · 2017-08-17T11:38:42Z

@bors try (for cargobomb)

bors · 2017-08-17T11:38:52Z

⌛ Trying commit cdfe8d7 with merge f6c35c6...

@nikomatsakis

Forward-compatibly deny drops in constants if they *could* actually run. This is part of #40036, specifically the checks for user-defined destructor invocations on locals which *may not* have been moved away, the motivating example being: ```rust const FOO: i32 = (HasDrop {...}, 0).1; ``` The evaluation of constant MIR will continue to create `'static` slots for more locals than is necessary (if `Storage{Live,Dead}` statements are ignored), but it shouldn't be misusable. r? @nikomatsakis

bors · 2017-08-17T13:05:46Z

☀️ Test successful - status-travis
State: approved= try=True

eddyb · 2017-08-17T13:08:35Z

cc @rust-lang/infra Cargobomb run requested.

aidanhs · 2017-08-18T18:10:26Z

Cargobomb run started

nikomatsakis · 2017-08-21T21:30:26Z

Any news from cargobomb run?

aidanhs · 2017-08-22T12:53:41Z

~27 hours remaining. I was away for the weekend so didn't manage to kick off a phase of the run until yesterday.

aidanhs · 2017-08-23T23:58:11Z

At long last - http://cargobomb-reports.s3.amazonaws.com/pr-43932/index.html

eddyb · 2017-08-24T07:27:38Z

Great, only regression is Rocket (which I've seen on crater before). However, I've changed by mind about it - it only uses associated const in inherent impls and we should probably treat those the same as const items - since there is no trait-like type dispatch being involved.

alexcrichton · 2017-08-24T14:25:42Z

Switching to S-waiting-on-review as looks like cargobomb's been run, cc @nikomatsakis

eddyb · 2017-08-24T17:16:24Z

Also waiting on me to treat inherent associated consts the same as const items.

eddyb · 2017-08-27T15:02:19Z

Latest crater report confirms Rocket has been fixed. r? @nikomatsakis

arielb1 · 2017-08-27T15:13:14Z

@eddyb

Could you add a test for the rocket case, so we can make sure it stays fixed?

nikomatsakis · 2017-08-28T15:48:19Z

src/librustc_mir/transform/qualify_consts.rs

+                // HACK(eddyb) Emulate a bit of dataflow analysis,
+                // conservatively, that drop elaboration will do.
+                let needs_drop = if let Lvalue::Local(local) = *lvalue {
+                    self.local_needs_drop[local]


I presume that we will reject (for constants) things that mutate "MIR locals" once they are assigned?

Yeah that is considered an assignment and disallowed as "statement-like".

nikomatsakis · 2017-08-28T15:50:39Z

src/test/compile-fail/static-drop-scope.rs

+    fn drop(&mut self) {}
+}
+
+static FOO: Option<&'static WithDtor> = Some(&WithDtor);


maybe add a test with struct WithoutDtor and Some(&WithoutDtor)? Does that behave differently?

That has worked since 1.0, but if you have something more advanced in mind that would actually stress the new code, I'm up for it.

nikomatsakis

Seems reasonable. I left a few questions.

nikomatsakis · 2017-08-28T15:51:30Z

(To be clear, I want to revisit shortly once I see answers.)

nikomatsakis · 2017-08-28T20:11:46Z

@eddyb would you mind adding a few more tests, just to have more "exhaustive" coverage of the various cases? otherwise, r=me I think.

nikomatsakis · 2017-08-30T16:24:36Z

@bors r+ -- after some discussion with @eddyb on IRC, decided tests are "good enough".

bors · 2017-08-30T16:24:37Z

📌 Commit c76a024 has been approved by nikomatsakis

bors · 2017-08-30T20:12:31Z

⌛ Testing commit c76a024 with merge 7eeac1b...

@nikomatsakis

Forward-compatibly deny drops in constants if they *could* actually run. This is part of #40036, specifically the checks for user-defined destructor invocations on locals which *may not* have been moved away, the motivating example being: ```rust const FOO: i32 = (HasDrop {...}, 0).1; ``` The evaluation of constant MIR will continue to create `'static` slots for more locals than is necessary (if `Storage{Live,Dead}` statements are ignored), but it shouldn't be misusable. r? @nikomatsakis

bors · 2017-08-30T22:59:27Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 7eeac1b to master...

@nikomatsakis

Better StorageLive / StorageDead placement for constants. Fixes problems in miri (see rust-lang/miri#324 (comment)) caused by the new scope rules in #43932. What I've tried to do here is always have a `StorageLive` but no `StorageDead` for `'static` slots. It might not work perfectly in all cases, but it should unblock miri. r? @nikomatsakis cc @oli-obk

eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Aug 17, 2017

rust-highfive assigned nikomatsakis Aug 17, 2017

arielb1 reviewed Aug 17, 2017

View reviewed changes

alexcrichton added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 24, 2017

eddyb added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 24, 2017

eddyb force-pushed the const-scoping branch 5 times, most recently from 52667d8 to 8697b80 Compare August 27, 2017 12:11

eddyb removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 27, 2017

eddyb force-pushed the const-scoping branch from 8697b80 to f5a22ea Compare August 28, 2017 05:12

eddyb added 3 commits August 28, 2017 09:27

rustc: use rvalue scope semantics for constant initializers.

b8c05fe

rustc_mir: conservatively deny non-noop drops in constant contexts.

9b61771

rustc: treat impl associated consts like const items for constness.

c76a024

eddyb force-pushed the const-scoping branch from f5a22ea to c76a024 Compare August 28, 2017 06:28

eddyb mentioned this pull request Aug 28, 2017

permit Drop types in constants (tracking issue for RFC #1440) #33156

Closed

2 tasks

nikomatsakis reviewed Aug 28, 2017

View reviewed changes

nikomatsakis added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2017

bors merged commit c76a024 into rust-lang:master Aug 30, 2017

bors mentioned this pull request Aug 30, 2017

Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. #44171

Merged

eddyb deleted the const-scoping branch August 31, 2017 05:10

eddyb mentioned this pull request Sep 1, 2017

Better StorageLive / StorageDead placement for constants. #44252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forward-compatibly deny drops in constants if they could actually run. #43932

Forward-compatibly deny drops in constants if they could actually run. #43932

eddyb commented Aug 17, 2017 •

edited

Loading

arielb1 Aug 17, 2017

eddyb Aug 17, 2017

eddyb commented Aug 17, 2017

bors commented Aug 17, 2017

bors commented Aug 17, 2017

eddyb commented Aug 17, 2017

aidanhs commented Aug 18, 2017

nikomatsakis commented Aug 21, 2017

aidanhs commented Aug 22, 2017

aidanhs commented Aug 23, 2017

eddyb commented Aug 24, 2017

alexcrichton commented Aug 24, 2017

eddyb commented Aug 24, 2017

eddyb commented Aug 27, 2017

arielb1 commented Aug 27, 2017

nikomatsakis Aug 28, 2017

eddyb Aug 28, 2017

nikomatsakis Aug 28, 2017

eddyb Aug 29, 2017

nikomatsakis left a comment

nikomatsakis commented Aug 28, 2017

nikomatsakis commented Aug 28, 2017

nikomatsakis commented Aug 30, 2017

bors commented Aug 30, 2017

bors commented Aug 30, 2017

bors commented Aug 30, 2017

Forward-compatibly deny drops in constants if they *could* actually run. #43932

Forward-compatibly deny drops in constants if they *could* actually run. #43932

Conversation

eddyb commented Aug 17, 2017 • edited Loading

arielb1 Aug 17, 2017

Choose a reason for hiding this comment

eddyb Aug 17, 2017

Choose a reason for hiding this comment

eddyb commented Aug 17, 2017

bors commented Aug 17, 2017

bors commented Aug 17, 2017

eddyb commented Aug 17, 2017

aidanhs commented Aug 18, 2017

nikomatsakis commented Aug 21, 2017

aidanhs commented Aug 22, 2017

aidanhs commented Aug 23, 2017

eddyb commented Aug 24, 2017

alexcrichton commented Aug 24, 2017

eddyb commented Aug 24, 2017

eddyb commented Aug 27, 2017

arielb1 commented Aug 27, 2017

nikomatsakis Aug 28, 2017

Choose a reason for hiding this comment

eddyb Aug 28, 2017

Choose a reason for hiding this comment

nikomatsakis Aug 28, 2017

Choose a reason for hiding this comment

eddyb Aug 29, 2017

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Aug 28, 2017

nikomatsakis commented Aug 28, 2017

nikomatsakis commented Aug 30, 2017

bors commented Aug 30, 2017

bors commented Aug 30, 2017

bors commented Aug 30, 2017

Forward-compatibly deny drops in constants if they could actually run. #43932

Forward-compatibly deny drops in constants if they could actually run. #43932

eddyb commented Aug 17, 2017 •

edited

Loading